Pull Request Review: Create python-app.yml

Thanks for this PR! This adds valuable CI/CD and deployment infrastructure. I've reviewed the changes and have feedback across several categories:

🟢 Strengths

Documentation

• Excellent deployment guide - docs/deployment/fly.md is comprehensive and well-structured with clear steps
• Quick start checklist - Makes deployment accessible to non-technical users
• README updates - Properly documents the new deployment capability

Architecture

• Smart default change - Switching from cloud to cpu mode keeps processing private by default
• Fallback handling - normalize_processing_mode() prevents accidental cloud API usage
• Good container design - Dockerfile includes necessary system dependencies for OCR

🟡 Issues & Recommendations

1. GitHub Actions Workflow (.github/workflows/python-app.yml)

Critical Issues:

• Missing dependency installation - The workflow installs from requirements.txt, but this project uses pyproject.toml
• Flake8 already in dev dependencies - No need to install separately (see pyproject.toml:69)
• Pytest already in dev dependencies - No need to install separately (see pyproject.toml:66)

Suggested fix:

- name: Install dependencies
  run: |
    python -m pip install --upgrade pip
    pip install -e ".[dev]"

Configuration Issues:

• Flake8 line length mismatch - Workflow uses max-line-length=127, but Black is configured for 88
• Missing quality tools - Project uses Black, isort, and mypy per CLAUDE.md, but CI only runs flake8

2. Dockerfile

Good practices:

• ✅ Slim base image
• ✅ Proper environment variables
• ✅ Cleans up apt cache
• ✅ Uses gunicorn for production

Issues:

• Gunicorn not in pyproject.toml - Should be in web extras for consistency
• Version pinning - Consider pinning gunicorn version for reproducibility
• Dependency installation - pip install .[web] only includes Flask; needs all core deps

3. Web App Changes (docstrange/web_app.py)

Good changes:

• ✅ normalize_processing_mode() provides input sanitization
• ✅ Changes default from cloud to cpu mode

Issues:

• Removed import - secure_filename was removed; verify this doesn't create a security issue
• Missing type hint - normalize_processing_mode() should have return type annotation
• Unused global - Global extractor is created but then overridden in requests

4. Fly.io Configuration (fly.toml)

Issues:

• No health checks - Consider adding HTTP health endpoint
• Cold starts - min_machines_running = 0 means first request will be slow
• Memory allocation - 4GB might not be enough for large documents with OCR

Suggestions:

• Add /health endpoint
• Document cold start behavior
• Consider min_machines_running = 1 for production

🔒 Security Considerations

1. File upload validation - Verify removal of secure_filename doesn't create vulnerability
2. ✅ Secrets management - Good use of flyctl secrets
3. ✅ HTTPS enforcement - force_https = true
4. ✅ Input sanitization - normalize_processing_mode() prevents injection
5. Consider adding - Rate limiting for API endpoint

📊 Test Coverage

Major Gap: No tests for new functionality

• ❌ No tests for normalize_processing_mode()
• ❌ No tests for web app with cpu mode
• ❌ No CI test of Dockerfile build
• ❌ No integration tests for API endpoint

✅ Action Items

Must Fix (Blocking):

1. Fix GitHub Actions workflow to use pyproject.toml instead of requirements.txt
2. Add tests for normalize_processing_mode()
3. Verify secure_filename removal doesn't create security vulnerability

Should Fix (Important):

4. Align flake8 config with Black's line length (88)
5. Add gunicorn to pyproject.toml web extras
6. Add health check endpoint for Fly.io
7. Add type hints to new functions

Nice to Have:

8. Add UI indicator for processing mode
9. Add rate limiting
10. Add Docker build test to CI
11. Add integration tests for API endpoint

📝 Summary

This PR adds valuable deployment infrastructure, but the GitHub Actions workflow won't work without fixing the dependency installation. The Dockerfile and Fly.io config are solid, and the security-conscious defaults (CPU mode by default) are excellent.

Recommendation: Request changes to fix the critical CI issues before merging.

Great work on the comprehensive deployment documentation! 🎉